Hey everyone!
I believe this should be a working saltclass. Please don't hesitate to point on my errors - I'll fix it as soon as possible.
@rbthomp @dwoz @mgomersbach @mchugh19 and others interested.

Next things to do:

• fix bugs you find :)
• update documentation a bit
• refactor find_and_process_re - didn't have time for this
• reintroduce globbing
• evaluate performance
• give an option to change class traverse mode in options (if class tree is constructed well and without hidden overlaps in pillar subtrees nobody will notice anything anyways)
• optional pretty output for __saltclass__ element in pillar.items (make it dict to look like tree, maybe?)
• saltclass matcher to address nodes in CLI by class names - have no idea if it's possible, but will look into it.

Wow, this is great! I'll definitely carve some time next week to test the PR.

+1 for the matcher

@max-arnold nice!
About CI - I looked through those 3-10M console outputs and didn't quite understand why they fail. It seems like saltclass is okay and it's other modules' issues (well, except linter checks).

This looks great already @a-a-abramov !

Just to keep thread alive, here's the list of fixed issues:
#51817 and #49175 - fixed
#51489 - fixed by removal of globbing for now, but I'm working on globbing overhaul right now, so it's due today-tomorrow
#51360 - now you can access __pillar__ dict from jinja in exactly the same manner you accessed __grains__, but with an important caveat - since you need pillars to render jinja while getting new pillars and changing old ones as the result of rendering, you can only access pillars from already rendered at that moment classes. It might sound a bit weird, but if you think more about this you would understand that it's classic chicken and egg problem. To clarify the method, here's a little example:

node:
  - A:
    - B
    - C:
      - D
  - E:
    - F
    - B

(implying node has classes A and E attached if it's not clear enough)
The order in which classes would be traversed is determined with BFS:
node, A, E, B, C, F, (B), D
Note, that second occurrence of B will not be processed. So what about __pillar__ dict for jinja? In each class you can use pillars, which were processed before. For example in B we can use pillars from node, A, E, but not from C, F, D. I'd also like to add configuration option to choose traversing option - BFS or DFS. For a well-constructed pillar tree it should not be an issue, but it's better to give user an option.
#50755 - fixed
#50265 - must be resolved, but could anyone check please? Unfortunately I don't have time to dive into integration test, but if the whole ext_pillar works as intended I did fix this.
#50262 - fixed
#48170 - 1 and 2 mentioned above and/or fixed before, surprisingly 3 works fine. 4 - didn't think about it, I suppose you're right about failing hard.
#48167 - I've already addressed issue with unnecessary filesystem searches locally and it works fine, though I didn't notice any significant performance boost. But neither did I run stress tests or profilers, so everything is possible :)

#50688, #50501 - didn't check yet
Several questions and feature requests - will look into those after this PR would be accepted, otherwise there's not point in doing so :)

@a-a-abramov Thanks for the commit and addressing the many issues. I've found a small bug, we need to account for files that are blank. It looks like render_yaml() returns a nonetype when nothing is rendered. I added a little logic to `get_class()' to check for this. Here is what I did to correct this.

    if straight in l_files:
        c_rendered  =  render_yaml(straight, salt_data)
    elif  sub_straight in l_files:
        c_rendered = render_yaml(sub_straight, salt_data)
    elif  sub_init in l_files:
        c_rendered = render_yaml(sub_init, salt_data)
    else:
        log.warning('%s: Class definition not found', _class)
        return {}

    # Verify rendered yaml is not blank.
    if c_rendered:
        return c_rendered

    return {}

Probably need to perform the same check on node.yml files.

Globbing is back! Though I need an additional approve from you on it since I changed a bit of how it works - will explain tomorrow.

So about globbing. I tried to make it intuitive and I think I succeeded. Basically it works like this:

• A.B.* will resolve into:
  • A.B.C
  • A.B.C.D
  • ... the whole subtree ...
  • A.B if A.B is defined with A/B/init.yml.

The last thing is questionable, but my team decided that it's a lesser evil, because otherwise if you have a number of files and folders under A/B, all of them would be included EXCEPT init.yml. That's exactly the unintuitivity I wanted to avoid. What do you think?

• .X.Y in A.B will resolve into A.B.X.Y - nothing special

• mixture of both works exactly like the mixture of both: .* will resolve into each "neighbour" class, "local" init.yml and the subtree

• One speciality is that . (single dot) includes local init.yml. If used in init.yml itself - safely ignored.

Order of classes from resolved glob is alphanumeric - I get absolute path for each class yml file, sort it and keep it ordered during resolution. My thought was that instead of implementating some complex logic I'd better keep it as simple and predictable as possible. Also I don't think that globbing for deep subtrees is a way to go for anyone.

Could anyone tell me what to do next? @Ch3LL? Failed tests confuse me a bit since I can't understand if it's my fault or not. It's also worth to notice that implementation from all the releases saltclass was in is severely bugged and unusable under any circumstances.

This PR would be completed in a few days when I overcome my procrastination and finish user manual.

Got some initial roadbumps during my testing (existing reclass-based state/pillar tree with obvious renames for saltclass):

% python --version
Python 2.7.15


% salt-call --local --id example.com pillar.data
[ERROR   ] Exception caught loading ext_pillar 'saltclass':
  File "/Users/user/.virtualenvs/test-salt/lib/python2.7/site-packages/salt/pillar/__init__.py", line 974, in ext_pillar
    key)
  File "/Users/user/.virtualenvs/test-salt/lib/python2.7/site-packages/salt/pillar/__init__.py", line 902, in _external_pillar_data
    *val)
  File "/Users/user/.virtualenvs/test-salt/lib/python2.7/site-packages/salt/pillar/saltclass.py", line 65, in ext_pillar
    return sc.get_pillars(minion_id, salt_data)
  File "/Users/user/.virtualenvs/test-salt/lib/python2.7/site-packages/salt/utils/saltclass.py", line 439, in get_pillars
    pillars, classes, states, environment = get_saltclass_data(node_data, salt_data)
  File "/Users/user/.virtualenvs/test-salt/lib/python2.7/site-packages/salt/utils/saltclass.py", line 300, in get_saltclass_data
    .format(dirpath, filename, dirpath, filename[:-4]))

[CRITICAL] Pillar render error: Failed to load ext_pillar saltclass: Conflict in class file structure - file ./reclass/classes/project/meta/example/test.yml and directory ./reclass/classes/project/meta/example/test.

The naming conflict above happens when there is a test.yml and test/ dir in the same directory. This worked just fine in reclass (if I remember correctly, test.yml is preferred over test/ + implicit init.yml).

% python --version
Python 3.6.6

% salt-call --local --id example.com pillar.data
[ERROR   ] Failed to import pillar saltclass, this is due most likely to a syntax error:
Traceback (most recent call last):
  File "/Users/user/.virtualenvs/test-salt3/lib/python3.6/site-packages/salt/loader.py", line 1548, in _load_module
    mod = spec.loader.load_module()
  File "<frozen importlib._bootstrap_external>", line 399, in _check_name_wrapper
  File "<frozen importlib._bootstrap_external>", line 823, in load_module
  File "<frozen importlib._bootstrap_external>", line 682, in load_module
  File "<frozen importlib._bootstrap>", line 265, in _load_module_shim
  File "<frozen importlib._bootstrap>", line 684, in _load
  File "<frozen importlib._bootstrap>", line 665, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 678, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/Users/user/.virtualenvs/test-salt3/lib/python3.6/site-packages/salt/pillar/saltclass.py", line 17, in <module>
    import salt.utils.saltclass as sc
  File "/Users/user/.virtualenvs/test-salt3/lib/python3.6/site-packages/salt/utils/saltclass.py", line 308
    salt_data['class_paths'] = OrderedDict(sorted(salt_data['class_paths'].iteritems(), key=lambda (k, v): k))
                                                                                                   ^
SyntaxError: invalid syntax
[CRITICAL] Specified ext_pillar interface saltclass is unavailable
'example.com' is not available.

On a positive note, refactored saltclass is able to detect duplicate keys:

parameters:
  network:
    interfaces:
    interfaces:
      eth0: 1.2.3.4

[ERROR   ] YAML rendering exception for file ./reclass/nodes/selectel/example.com.yml:
while constructing a mapping
  in "<unicode striding>", line 44, column 5:
        interfaces:
        ^
found conflicting ID 'interfaces'
  in "<unicode string>", line 45, column 5:
        interfaces:
        ^
[WARNING ] Unable to render yaml from ./reclass/nodes/selectel/example.com.yml

Tested against a-a-abramov@2b8da76

This PR would be completed in a few days when I overcome my procrastination and finish user manual.

You could probably borrow some useful bits from the release notes: https://docs.saltstack.com/en/latest/topics/releases/2018.3.0.html#new-saltclass-pillar-master-tops-modules

@max-arnold
As you can probably see I raise conflict error intentionally. I think reclass'es model with priority of one file over the other doesn't really give you benefits but it's ambiguity could lead to unintuitive pillar rendering.I believe we should keep it simple and fail hard.

And for the next error - good point, I completely forgot to test the module against python3. Will do it asap.

Performance tests (#48167):

2018.3.4:

% time salt-call --local --id example.com pillar.data > /dev/null
salt-call --local --id example.com pillar.data > /dev/null  2,52s user 1,30s system 91% cpu 4,155 total

Profiling data (started at salt/utils/saltclass.py:get_pillars):

2019.2@2b8da76:

% time salt-call --local --id example.com pillar.data > /dev/null
salt-call --local --id example.com pillar.data > /dev/null  1,58s user 0,74s system 87% cpu 2,647 total

Profiling data (started at salt/utils/saltclass.py:get_pillars):

The second graph looks much more dense, with the majority of time spent in yaml/composer.py:compose_node. Maybe there is some minor optimization potential in blue _compile box (bottom-left), related to regexp compilation.

Steps to reproduce:

% find reclass/classes -type f | wc -l
     285

% find reclass/nodes -type f | wc -l
      56

python -m cProfile -o salt.profile ~/.virtualenvs/test-salt/bin/salt-call --local --id example.com pillar.data
pyprof2calltree -i salt.profile -o salt.calltree

The resulting salt.calltree could be opened with kcachegrind, qcachegrind or anything from this list on a Mac: https://stackoverflow.com/questions/5426799/is-there-any-kcachegrind-alternative-for-mac-os-x-outta-there

Overall verdict: FIXED!

Other issues are left for tomorrow.

Fixed 2to3 issue

@a-a-abramov Great work here but this needs to go to the develop branch instead of 2019.2

Great work here but this needs to go to the develop branch instead of 2019.2

@dwoz This is a bugfix (albeit a large one). Saltclass is plagued by multiple problems from the moment it was merged in 2018.3 without proper QA (see the list of issues that is fixed by this PR).

Things got even worse in 2019.2.0 - Saltclass is completely broken and doesn't work at all for many people, plus the original author is unresponsive for several months:

It is unpractical to fix these bugs one-by-one because the implementation has too many defects and it is much simpler to just rewrite it.

By rebasing this PR on top of the develop branch, you are effectively delaying those necessary fixes until the Neon release, which is not yet scheduled and won't be shipped in several months. This leaves Saltclass implementation in a broken state.

Since it is hard to break backward compatibility for the already broken feature, and the feature is experimental (it doesn't even have proper docs and only mentioned in 2018.3 release notes), please consider reverting your decision and allow us to rectify this mess in the nearest bugfix release (2019.2.1 or 2019.2.2)

@max-arnold Thanks for explaining this more. We'll still need to make sure the linter, docs, and PR tests all pass to get this into 2019.2.1.

@dwoz Thanks for understanding!

How much time do we have to finalize the PR before 2019.2.1 is branched?

Can someone decypher the lint error? Because I seem to be reading over it.

Hello,

Without wanting to add a mess to the mess I customized saltclass for my own need. Maybe I should wait for this to be merged first but here are the general ideas:

• Can use default classes from master configuration (no need to create a node)
• Allow additional filters such as serialization filters and is_dict
• Allow Depth-First search instead of Breath-First for classes (probably I should change the behavior based on salt master configuration as suggested in the current thread).

Currently the changes are limited regarding the current pull request, but I would prefer the current to be merged before adding more stuff. More especially I have no test cases :-(.

What do you suggest?

@ptitdoc this PR is pointing to a branch that is no longer being updated or supported. It will not be merged unless it is rebased and pointing to the current master branch and tests will be required to be merged. We are considering closing this PR.

@sagetherage If this is closed, where does that leave SaltClass as part of SaltStack? At the very least I think the closure of this PR should lead to some clarification on that front, since as it stands it isn't really usable.

@sagetherage If this is closed, where does that leave SaltClass as part of SaltStack? At the very least I think the closure of this PR should lead to some clarification on that front, since as it stands it isn't really usable.

@afletch this is only a PR that is pointing to the incorrect branch, is the reason for the closure. Work can still be accomplished and I can re-open, but someone from the community will need to commit to the work, rebase this against master and write tests.

@sagetherage sorry, I was speaking about the PR as a whole, which it seems has stalled since @a-a-abramov no longer has the time to work on it. This PR was the 'hope for SaltClass' and without it - as even @a-a-abramov notes - SaltClass as distributed in SaltStack, is pretty much unusable. Therefore, my point was; if this PR is closed it signifies that SaltClass as pretty much abandoned. I raised this in Open Hour a few months back and there seemed little appetite beyond "someone needs to pick it up". Perhaps it should be removed (though I appreciate that's a discussion for elsewhere, not this PR).
edit: that would be a shame, as SaltClass had great promise.

@sagetherage sorry, I was speaking about the PR as a whole, which it seems has stalled since @a-a-abramov no longer has the time to work on it. This PR was the 'hope for SaltClass' and without it - as even @a-a-abramov notes - SaltClass as distributed in SaltStack, is pretty much unusable. Therefore, my point was; if this PR is closed it signifies that SaltClass as pretty much abandoned. I raised this in Open Hour a few months back and there seemed little appetite beyond "someone needs to pick it up". Perhaps it should be removed (though I appreciate that's a discussion for elsewhere, not this PR).
edit: that would be a shame, as SaltClass had great promise.

@afletch I hear you! I have also tried to find someone that is willing to even have this discussion! you can find me on saltstackcommunity.slack.com or keybase or IRC all with the same handle as here or you can email me [email protected] if you want to discuss further. I had someone in mind for this work but that fell through, recently. Still looking.

Hey everyone!

First of all I'm extremely sorry for abandoning this PR. At first I did not expect that simple rewriting and adding a couple of minor features would unravel real issues with the original code. And after a couple of weeks of struggling through the reviews (kudos to everyone involved!) tasks at work began to pile up with astounding rate and I had neither time and energy for this nor courage to let you guys know :(

But! I left my job a couple of weeks before and now I have both time and energy to finally invest into saltclass. But I saw a couple of PRs featuring saltclass and don't really know if there's anything left to improve. If there is - let me know! I also think that we should first agree on the scope here. Ideally gather some feedback and/or requests from both the community and salt team.

@a-a-abramov wondering if this may be a good candidate for a [salt-extension](Reviewed in Grooming 2021-MAY-03 closing), unsure, merely an idea.

Are there any updates regarding this issue?

But I saw a couple of PRs featuring saltclass and don't really know if there's anything left to improve. If there is - let me know! I also think that we should first agree on the scope here. Ideally gather some feedback and/or requests from both the community and salt team.

Did you want to close this PR and open a new one when issues have been created around the other issues?

@sagetherage 's suggestion is also a good one. If someone in the community is willing to maintain it we could create a saltclass pillar salt-extension project (https://github.com/saltstack/salt-extension) This would allow this module to be managed and release outside of Salt.

Is anyone willing to follow through with this PR?

I'm not sure why this couldn't be merged as-is. It fixes a lot of outstanding issues. I know there were some ideas of adding more bells and whistles to it, but for the basic functionality this patch is already way more stable and functional than what currently exists.

I would really like to see this merged, I use saltclass to manage orchestration in testing envrionments, and I don't know of any other solution within salt that would allow effective management of large numbers of nodes that need lots of pillar data associated with them.

The most recent comment from @a-a-abramov led me to belive there are still issues that need to be resolved.

Also this will require a changelog to be added and it needs to be submitted against the master branch not 2019.2.

Closing this PR due to inactivity and the target branch is wrong. If anyone wants to continue this PR, please open a new PR against the master branch with the changes and we can continue progressing these changes.